Support Slashes in Feature Names in UI/API #362
Conversation
refs #357 This is a tricky one. The issue is that I was assuming no one would use slashes. Using a slash completely broke all the routing. I changed the routing to be a block so it is more flexible. Now it is possible to route based on anything in request. The API will likely need the same work.
| it 'renders add new actor form' do | ||
| form = '<form action="/features/a/b/actors" method="post" class="form-inline">' | ||
| expect(last_response.body).to include(form) | ||
| end |
| @feature_name ||= Rack::Utils.unescape(path_parts[-2]) | ||
| @feature_name ||= begin | ||
| match = request.path_info.match(REGEX) | ||
| match ? match[1] : nil |
There was a problem hiding this comment.
super minor suggestion these regexes could use named capture groups that might read slightly better, but this also works great as is
REGEX = %r{\A/features/(?<feature_name>.*)/actors/?\Z}
def feature_name
if match = request.path_info.match(REGEX)
match[:feature_name]
end
end
There was a problem hiding this comment.
I almost went for a capture group, but this took so much longer than I was planning I lost steam. I think it is worth updating to use them so I'll spin back around on it soon.
There was a problem hiding this comment.
Updated in f61bbe9
There was definitely a reason in the beginning that had to do with escaping/matching regex, but I wonder if at some point I worked around that and never went back. I'll look at swapping it out again and see how that works. |
We just need to match the path info to a regex. We don't need the whole request.
|
Moved back to single regex in 8fa06a5. Going to get rid of the constants too. |
|
Ok, waiting for CI but I think this is ready and I'm going to merge soon. |
As brought up in #357, flipper UI (and API) do not handle slashes in feature names. This fixes that. I had to redo routing stuff, but it should all work the same.
I considered adding a note that slashes aren't supported, but it wasn't too bad to add support so why not. There is some duplication in finding the feature name, but I didn't feel like there was enough to warrant any abstracting just yet. We'll see if this churns at all and would benefit from that down the road.
fixes #357